-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Backport 3.6: Incremental TLS handshake defragmentation #10030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport 3.6: Incremental TLS handshake defragmentation #10030
Conversation
dd3e3f5 to
7988f71
Compare
Pacify `clang -Wdocumentation`. Signed-off-by: Gilles Peskine <[email protected]>
In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "waiting for more handshake fragments" log message in ssl_consume_current_message(), and add a similar one in mbedtls_ssl_prepare_handshake_record(). Assert both. Signed-off-by: Gilles Peskine <[email protected]>
In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak the "handshake fragment:" log message. This changes what information is displayed when a record contains data beyond the expected end of the handshake message. This case is currently untested and its handling will change in a subsequent commit. Signed-off-by: Gilles Peskine <[email protected]>
Minor refactoring of the initial checks and preparation when receiving the first fragment. Use `ssl->in_hsfraglen` to determine whether there is a pending handshake fragment, for consistency, and possibly for more robustness in case handshake fragments are mixed with non-handshake records (although this is not currently supported anyway). Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Reassemble handshake fragments incrementally instead of all at the end. That is, every time we receive a non-initial handshake fragment, append it to the initial fragment. Since we only have to deal with at most two handshake fragments at the same time, this simplifies the code (no re-parsing of a record) and is a little more memory-efficient (no need to store one record header per record). This commit also fixes a bug. The previous code did not calculate offsets correctly when records use an explicit IV, which is the case in TLS 1.2 with CBC (encrypt-then-MAC or not), GCM and CCM encryption (i.e. all but null and ChachaPoly). This led to the wrong data when an encrypted handshake message was fragmented (Finished or renegotiation). The new code handles this correctly. Signed-off-by: Gilles Peskine <[email protected]>
Pacify `clang -Wformat-pedantic`. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
7988f71 to
e6eed84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Failthfull backport which contains a single MSCV fix not included in the development PR.
The pointer to the framework may need to be updated before merging, since we are failing component_test_tls13_only_psk because all tests are being skipped.
e6eed84 to
baaccb3
Compare
Changed log messages and added more tests in `tests/opt-testcases/handshake-generated.sh`. Signed-off-by: Gilles Peskine <[email protected]>
baaccb3 to
7719169
Compare
Signed-off-by: Gilles Peskine <[email protected]>
A handshake record may contain multiple handshake messages, or multiple fragments (there can be the final fragment of a pending message, then zero or more whole messages, and an initial fragment of an incomplete message). This was previously untested, but supported, so don't break it. Signed-off-by: Gilles Peskine <[email protected]>
There is no longer any different processing at this point, just near-identical log messages. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
mpg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm satisfied this is a faithful backport with the name and type of in_hsfraglen changed.
git range-diff wasn't as useful as it usually is, so this time I just took the whole diff for dev and 3.6, did s/badmac_seen_or_in_hsfraglen/in_hsfraglen/ in the 3.6 diff (and s/^@@ [^ ]* [^ ]* @@/@@/ in both to reduce noise), before diffing the diffs. All that was left was differences related to the type change, which all looked OK.
Signed-off-by: Gilles Peskine <[email protected]>
mpg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
The CI is still failing at |
|
@minosgalanakis Not all tests skipped, there's one test case that's failing, "Connection ID, 3D+MTU: Cli+Srv enabled, CID on renegotiation", and that's a known intermittent failure. After discussion on Slack, let's just go ahead and merge. Note that we had a pass before c22e315, and an almost-pass after. |
26c378c
into
Mbed-TLS:features/tls-defragmentation/3.6
Backport of #10011. Differences:
size_t ssl->in_hsfraglenbecomesunsigned ssl->badmac_seen_or_in_hsfraglen: name change and printf specifier changes.PR checklist